Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move commitment verification into verifier #1051

Closed
wants to merge 1 commit into from

Conversation

litt3
Copy link
Contributor

@litt3 litt3 commented Dec 20, 2024

This PR moves the newly implemented commitment verification into the verifier, as suggested here

@litt3 litt3 requested review from samlaf and cody-littley December 20, 2024 17:44
@litt3 litt3 self-assigned this Dec 20, 2024
Copy link
Contributor

@samlaf samlaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let's just make sure to make the methods private IF they don't need to be public, to not clutter the verifier's API. Not sure how/where you plan to use them so fine with me if they need to be public.

@@ -329,3 +329,45 @@ func PairingsVerify(a1 *bn254.G1Affine, a2 *bn254.G2Affine, b1 *bn254.G1Affine,

return nil
}

// GenerateBlobCommitment computes a kzg-bn254 commitment of blob data using SRS
func (v *Verifier) GenerateBlobCommitment(blob []byte) (*encoding.G1Commitment, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any particular external use cases in mind. My tendency would be to keep it public, since it's a general purpose method to compute commitment from bytes, with likelihood of being used elsewhere in the future. But I'd have no strong objection to making it private

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rework the API later but I find it a bit weird that this method is on a strict called Verifier. I might want to compute a kzg commitment without verifying anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. We could create a new type Committer, which would accept a kzg configuration, and just produce the commitments.


// GenerateAndCompareBlobCommitment generates the kzg-bn254 commitment of the blob, and compares it with a claimed
// commitment. An error is returned if there is a problem generating the commitment, or if the comparison fails.
func (v *Verifier) GenerateAndCompareBlobCommitment(claimedCommitment *encoding.G1Commitment, blobBytes []byte) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this will be called by the v2 client

@litt3
Copy link
Contributor Author

litt3 commented Dec 20, 2024

Closing this PR, to move the methods to better places. See #1051 (comment)

@litt3 litt3 closed this Dec 20, 2024
@litt3 litt3 deleted the move-commitment-verification branch January 3, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants